Skip to content

Conversation

@jordan-powers
Copy link
Contributor

This patch adds the needed data generator and source matcher to include counted_keyword fields in our randomized testing.

This patch also updates the source matcher such that field-specific matchers are checked before the generic matcher is used. It seems that this is the correct behavior, and the only reason the generic matcher was checked first was as a workaround for issue #111916, which is now closed.

@jordan-powers jordan-powers added >test Issues or PRs that are addressing/adding tests auto-backport Automatically create backport pull requests when merged :StorageEngine/Mapping The storage related side of mappings v8.19.0 v9.1.0 labels Jan 31, 2025
@jordan-powers jordan-powers requested a review from lkts January 31, 2025 21:38
@jordan-powers jordan-powers self-assigned this Jan 31, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)


this.valueGenerator = arrays.wrapper().compose(nulls.wrapper()).apply(() -> {
if (previousStrings.size() > 0 && ESTestCase.randomBoolean()) {
return ESTestCase.randomFrom(previousStrings);
Copy link
Contributor

@lkts lkts Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not directly use ESTestCase here, it should be hidden inside DataSource. You can probably define a generic "repeating" wrapper but i haven't looked too closely.

}
}

return counts.isEmpty();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

jordan-powers added a commit that referenced this pull request Feb 4, 2025
As a result of the randomized testing enabled in #121462, we found that
we currently fail to parse documents with arrays of objects containing
counted_keyword fields. This PR fixes this issue by using a custom
docvalues field to store the count instead of the built-in lucene
BinaryDocValues. This custom CountsBinaryDocValuesField has logic to
handle multiple values for the same field.
@jordan-powers jordan-powers merged commit 061920b into elastic:main Feb 4, 2025
17 checks passed
@jordan-powers jordan-powers deleted the counted_keyword_randomized_testing branch February 4, 2025 21:06
jordan-powers added a commit to jordan-powers/elasticsearch that referenced this pull request Feb 4, 2025
This patch adds the needed data generator and source matcher to include
counted_keyword fields in our randomized testing.

This patch also updates the source matcher such that field-specific
matchers are checked before the generic matcher is used. It seems that
this is the correct behavior, and the only reason the generic matcher was
checked first was as a workaround for issue elastic#111916, which is now closed.

(cherry picked from commit 061920b)
@jordan-powers
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.x

Questions ?

Please refer to the Backport tool documentation

elasticsearchmachine pushed a commit that referenced this pull request Feb 4, 2025
As a result of the randomized testing enabled in #121462, we found that
we currently fail to parse documents with arrays of objects containing
counted_keyword fields. This PR fixes this issue by using a custom
docvalues field to store the count instead of the built-in lucene
BinaryDocValues. This custom CountsBinaryDocValuesField has logic to
handle multiple values for the same field.

(cherry picked from commit 13b743c)
elasticsearchmachine pushed a commit that referenced this pull request Feb 5, 2025
This patch adds the needed data generator and source matcher to include
counted_keyword fields in our randomized testing.

This patch also updates the source matcher such that field-specific
matchers are checked before the generic matcher is used. It seems that
this is the correct behavior, and the only reason the generic matcher was
checked first was as a workaround for issue #111916, which is now closed.

(cherry picked from commit 061920b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :StorageEngine/Mapping The storage related side of mappings Team:StorageEngine >test Issues or PRs that are addressing/adding tests v8.19.0 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants